-
-
Notifications
You must be signed in to change notification settings - Fork 545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added CTA And Made Sidebar take whole space #1527
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Souravvmishra, for creating this pull request and contributing to LinksHub! 💗
The maintainers will review this Pull Request and provide feedback as soon as possible! 😇
We appreciate your patience and contribution, Keep up the great work! 😀
@rupali-codes Reviews are appreciated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not see the changes when I went to the preview site @Souravvmishra(see screenshot below). Can you fix this?
This is because it is available only on small screens devices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@rupali-codes @CBID2 as far my experience it's not that much suitable to add full side panel that covers the whole screen as you can see this big brands also use the 70% ration of the hamburger menu...... |
Did you do it on a small screen @k-deepak04? That's what it's for. |
yep christy all the ss are from small screens only |
Ah ok. It seemed to work on my phone. |
@Souravvmishra hi thanks. Can you make it for mobile screens only? not for tablet and all? |
Its build as per the design when the sidebar becomes hidden, we show a explore button. |
These Big Brands are using the sidebar as a sidebar for extra options but in our case, our sidebar has all the Focus of the user |
okay man :) |
Its build as per the design when the sidebar becomes hidden, we show a explore button. Are You Sure You Want To Remove It For Tablets? |
can you please merge this |
@Souravvmishra can we do it only for mobile screens? not for tablets and all? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be for small screen devices only.
Also, can you explain why full screen sidebar is better? I don't want assumptions. Say from the perspective of a user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Souravvmishra please tell why full screen is needed , as rightly said by anmol also have you did only for mobile not for tabs as requested by rupali?
If not please do it
@Souravvmishra |
Apologies for the late reply, I am on a trip. Why do we need a full screen? Actually I would Suggest Creating a new page instead of just using the sidebar These all work for small screens only as in the large screen the sidebar is already visible. So at least the user is aware of it. But in small screen, the user has no idea that they are in sidebar. Yes it is written but who reads? |
Feel free to reply whenever you have time. No rush! I don't understand this:
@rupali-codes @CBID2 @k-deepak04 |
I'm lost too @Anmol-Baranwal |
I think we should have one landing page then one for the categories from where user can go to the resources. |
@k-deepak04 yes we did, but still, how we actually going render resources in there? its not like 5-10 resources that user can directly see on the screen, they will have to scroll. And having a landing page is completely a sperate idea, it was for where we can show other things, like About LinksHub, Contacting us, How people can contribute, showing contributors list etc etc. Having the sidebar is not a problem, we just need to make it more accessible. @Anmol-Baranwal & @CBID2 what are your takes on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Souravvmishra its showing 2 scrollbars in the sidebar, could you please check?
@rupali-codes Later, someone must have changed the code which removed that functionality. See this: https://linkshub-1arqvr3it-rupali-codes.vercel.app/ Like, I did it properly. |
I agree @rupali-codes |
How are we going to move forward with this @rupali-codes and @Anmol-Baranwal? |
Don't know, @rupali-codes must be active. |
can we just keep this PR to add CTA and keep the sidebar as it is currently? |
That's not a bad idea @rupali-codes. What do you think @Anmol-Baranwal? |
Closed due to inactivity |
Fixes
Closes #1448
Changes proposed
Screenshots
Note to reviewers